Hey, this is going in the right direction, but there should be a way to simplify the implementation by not introducing separate async tasks for inbound/outbound substreams and not adding command channels. It should be possible to implement polling of inbound substreams by putting them into FuturesUnordered or, may be, tokio_stream::StreamMap. The latter should be quite easy as there is maximum one inbound substream allowed from any remote peer as per ping spec: https://github.com/libp2p/specs/blob/master/ping/ping.md

tokio_stream::StreamMap might not work because we need to also write to substream and not only poll it, but still there should be a way to simplify the code.

Hey, this is going in the right direction, but there should be a way to simplify the implementation by not introducing separate async tasks for inbound/outbound substreams and not adding command channels. It should be possible to implement polling of inbound substreams by putting them into FuturesUnordered or, may be, tokio_stream::StreamMap. The latter should be quite easy as there is maximum one inbound substream allowed from any remote peer as per ping spec: https://github.com/libp2p/specs/blob/master/ping/ping.md

tokio_stream::StreamMap might not work because we need to also write to substream and not only poll it, but still there should be a way to simplify the code.

@dmitry-markin can you check my current implementation

Hey hey, nice use of SplitStream/SplitSink! I think what is missing is a cleaner separation between inbound and outbound substreams. On inbound substreams we should only respond to incoming pings, and on outbound send pings ourself. It would be better than relying on the presence of the time in the map, which can get screwed when we both send pings to a peer and receive pings from it.

i have done this now @dmitry-markin

Out of scope for this PR but I noticed that ping::config::MAX_FAILURES is not used anywhere. Should we create an issue for that?

@lexnv @haikoschol @dmitry-markin please help review

@lexnv @haikoschol @dmitry-markin please help review

LGTM, but my approval doesn't count towards mergeability.

Sorry for huge delay with reviewing. I have finally looked into the PR and tried to make it properly register protocols with appropriate keep-alive settings (dharjeezy-ping-keep-alive...dm-ping-keep-alive), but it turned out to be more tricky than I thought. The connection keep-alive mechanism relied on the fact that ping substreams were periodically opened, because the actual connection tear down happened only when somebody tried to open a new substream. With reused substreams it doesn't work anymore.

I am going to look further into this. Meanwhile, please don't push new changes to the PR. Thanks for all the work done so far!

I am going to look further into this. Meanwhile, please don't push new changes to the PR. Thanks for all the work done so far!

FWIW, after testing with a full smoldot light client (as opposed to our minimal test env), I am pretty sure that merging this would increase stability of connections from smoldot to full nodes. I've included the changes in this PR in my branch that I've used for testing.

FWIW, after testing with a full smoldot light client (as opposed to our minimal test env), I am pretty sure that merging this would increase stability of connections from smoldot to full nodes. I've included the changes in this PR in my branch that I've used for testing.

Yes, smoldot needs a permanent ping substream (as per libp2p ping spec), or will disconnect litep2p on a second ping attempt trying to write to substream that was reset. This is why this issue is of a higher priority now.